fix(mcp): propagate cfg.DefaultTenant to MCP fallback (RAN-22)#33
Merged
Conversation
Startup never called mcpServer.SetDefaultTenant, so header-less MCP
requests fell back to the hardcoded "default" tenant via
storage.DefaultTenantID -- ignoring DEFAULT_TENANT in deployments that
override it. Wire cfg.DefaultTenant in at MCP construction and surface
the resolved value in the startup log line.
Add an HTTP-transport unit test (TestSetDefaultTenant_PropagatesToHTTPTransport)
that exercises the actual fallback path through ServeHTTP: it asserts
the constructor default, the no-op semantics of SetDefaultTenant(""),
and that a header-less tools/call for find_similar_logs scopes to the
configured tenant. The explicit-header path is also re-checked to
confirm precedence (header wins over default) was not inverted.
The startup vector hydration concern in the original ticket is moot:
RAN-20 replaced the bare-appCtx tenant-scoped GetLogsV2 call with
ListRecentHighSeverityLogsAllTenants, which is cross-tenant by design
and tags each row with its own TenantID via vectorIdx.Add.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
…nforced Address review feedback on RAN-22 (PR #33): the previous test only exercised SetDefaultTenant, which existed before this branch -- so removing the main.go wiring would not have failed any test. Move defaultTenant from a post-construction setter to a required leading parameter on mcp.New. This turns the wiring contract into a compile-time check: removing or omitting cfg.DefaultTenant in main.go is now a build failure, not a silent regression. The runtime SetDefaultTenant setter is retained for future config-reload paths. Tests: - TestNew_DefaultTenant_FromConstructor exercises the constructor's fallback semantics (empty -> storage.DefaultTenantID; non-empty preserved verbatim) and confirms SetDefaultTenant runtime override still works without clobbering on empty input. - TestNew_DefaultTenant_FlowsThroughHTTPTransport drives the JSON-RPC HTTP handler with no X-Tenant-ID header on a server built via New("acme", ...) and asserts acme-scoped results, header precedence over the default, and that SetDefaultTenant runtime override flows through the same transport path. internal/mcp/tenant_isolation_test.go updated for the new signature (passes "" to preserve existing storage.DefaultTenantID semantics). Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cfg.DefaultTenantinto the MCP server at startup viaSetDefaultTenant. Without this, header-less MCP requests fell back to the hardcoded"default"(storage.DefaultTenantID) and ignored deployments that overrideDEFAULT_TENANT.ServeHTTP: asserts the constructor default, the no-op semantics ofSetDefaultTenant(""), that a header-lessfind_similar_logscall scopes to the configured tenant, and that an explicitX-Tenant-IDheader still wins over the configured default.The original ticket also flagged a startup vector-hydration site (
main.go:337using a bareappCtx). That concern is moot post-RAN-20: hydration now usesListRecentHighSeverityLogsAllTenantswhich is cross-tenant by design and tags each row with its ownTenantIDviavectorIdx.Add. No second fix needed; documented in the commit body.Resolves RAN-22. Builds on RAN-19 and RAN-20.
Test plan
go vet ./...cleango buildcleango test -count=1 -v -run TestSetDefaultTenant_PropagatesToHTTPTransport ./internal/mcp/...— PASSgo test -count=1 ./internal/mcp/... ./internal/api/... ./internal/vectordb/... ./internal/storage/...— all PASSTestFindSimilarLogs_TenantIsolation,TestFindSimilarLogs_NoTenantFallsBackToDefault,TestMCP_TenantIsolation_AllGraphRAGTools,TestMCP_TenantIsolation_DrainClusterIDsStayPerTenant— all PASSDEFAULT_TENANT=acme, hitPOST /mcpwith noX-Tenant-IDfor a tools/call, confirm response is acme-scoped and the startup log line showsdefault_tenant=acme.🤖 Generated with Claude Code